Skip to content

Add #parse_type#11126

Merged
straight-shoota merged 14 commits intocrystal-lang:masterfrom
Blacksmoke16:parse-type-name
Jan 21, 2022
Merged

Add #parse_type#11126
straight-shoota merged 14 commits intocrystal-lang:masterfrom
Blacksmoke16:parse-type-name

Conversation

@Blacksmoke16
Copy link
Copy Markdown
Member

@Blacksmoke16 Blacksmoke16 commented Aug 24, 2021

First pass at StringLiteral#parse_type_name. One thing I realized is this also allows resolving constants, but that's probably fine?

Resolves #11117

Comment thread src/compiler/crystal/macros/methods.cr Outdated
@HertzDevil
Copy link
Copy Markdown
Contributor

We should additionally disallow any extra unparsed tokens before or after the type name.

@Blacksmoke16
Copy link
Copy Markdown
Member Author

Do we think using parser.check :EOF is sufficient, or do we want to customize the error a bit? Currently it would raise like expecting token 'EOF', not '100' if you tried to do Bar(Int32)100. But it would raise expecting token: 100 if you tried to parse 100Foo.

@straight-shoota
Copy link
Copy Markdown
Member

Maybe we can just rescue any parse exception an raise a new error that just states that it's an invalid type name. Perhaps the original message can be attached for context.

@Blacksmoke16
Copy link
Copy Markdown
Member Author

@straight-shoota Yea that was a good call. Also makes it possible to include the invalid type name in the exception message and properly points to the call site.

Comment thread src/compiler/crystal/macros/methods.cr Outdated
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@Sija
Copy link
Copy Markdown
Contributor

Sija commented Aug 25, 2021

Needs a rebase after #10498

@Sija
Copy link
Copy Markdown
Contributor

Sija commented Aug 25, 2021

What about making it a top-level method?

@straight-shoota
Copy link
Copy Markdown
Member

Yes, I was going to ask about that, too. cf #11117 (comment)

@Blacksmoke16
Copy link
Copy Markdown
Member Author

Fine with me. However, is the expectation that parse_type would still return a Path or Generic (Union)? Or would we expect it to return a TypeNode by default now?

Also, I'm assuming this should be restricted to StringLiteral, and probably MacroId, given a SymbolLiteral would be kinda odd/nor could you create one.

@straight-shoota
Copy link
Copy Markdown
Member

Semantics should stay the same, i.e. return Path | Generic.
Restriction to StringLiteral and MacroId sounds fine.

@Blacksmoke16 Blacksmoke16 changed the title Add StringLiteral#parse_type_name Add #parse_type Aug 26, 2021
Comment thread src/compiler/crystal/macros.cr Outdated
Comment thread src/compiler/crystal/macros.cr Outdated
@Blacksmoke16
Copy link
Copy Markdown
Member Author

Anything left to do here?

@HertzDevil
Copy link
Copy Markdown
Contributor

Might want to add test cases for ProcNotation and Metaclass too (however you cannot resolve them yet, pending #11373 and #11375).

Also it is worth noting that technically Self, TypeOf, Splat, and Underscore nodes may be returned here; although the type grammar permits them, TypeOf is allowed only in normal code, whereas (top-level) Splat and Underscore are allowed only in restrictions, so calling #resolve on the returned node is not guaranteed to be meaningful.

@Blacksmoke16
Copy link
Copy Markdown
Member Author

Bump. Another review would be 🙏.

Comment thread src/compiler/crystal/macros.cr Outdated
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Comment thread src/compiler/crystal/macros/methods.cr Outdated
Comment thread src/compiler/crystal/macros.cr Outdated
# {{ parse_type("MissingType").resolve }} # => Error: undefined constant MissingType
# {{ parse_type("UNKNOWN_CONST").resolve }} # => Error: undefined constant UNKNOWN_CONST
# ```
def parse_type(type_name : StringLiteral) : Path | Generic
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this comment:

#11126 (comment)

it might be better to say that this returns ASTNode.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, thoughts on just adding them to the union to be more clear what it could return?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a minor thing, so we can merge this right away and always improve the docs later on.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already had it done, plus addressed your other comment, so I just pushed it up. Should be good to go now.

Don't skip `;;` when parsing type name
Copy link
Copy Markdown
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small suggestions, otherwise looks good 👍

Comment thread src/compiler/crystal/macros.cr
Comment thread src/compiler/crystal/macros/methods.cr Outdated
Blacksmoke16 and others added 2 commits January 14, 2022 09:30
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@straight-shoota straight-shoota added this to the 1.4.0 milestone Jan 14, 2022
@straight-shoota straight-shoota merged commit cd1441d into crystal-lang:master Jan 21, 2022
@Blacksmoke16 Blacksmoke16 deleted the parse-type-name branch January 21, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support resolving a StringLiteral into a TypeNode

5 participants